feat: Surface parameters fields for database model #14646
Conversation
| host: string; | ||
| password?: string; | ||
| port: number; | ||
| query: Object; |
There was a problem hiding this comment.
I think it is better to use the query props here, which I think you can import.
There was a problem hiding this comment.
Confusingly, this is a different query: https://docs.sqlalchemy.org/en/14/core/engines.html#add-parameters-to-the-url-query-string
There was a problem hiding this comment.
oh interesting, so then would Record<string, string> be more explicit here given that it has string keys and values?
There was a problem hiding this comment.
I also added in some props in #14583. For bigquery most of those props are going be optional (maybe some other dbs too). Are we going to use db parameters for Gsheets for example?
There was a problem hiding this comment.
@AAfghahi right, Record<string, string>
But we need to keep in mind that these are specific for Postgres and similar databases. For other DBs some attributes might be optional, or there might be a different set of attributes, so maybe we should just define the type as Object.
There was a problem hiding this comment.
that's fair, I hadn't considered that.
Codecov Report
@@ Coverage Diff @@
## master #14646 +/- ##
==========================================
+ Coverage 77.20% 77.59% +0.39%
==========================================
Files 958 958
Lines 48492 49127 +635
Branches 5691 5767 +76
==========================================
+ Hits 37437 38119 +682
+ Misses 10855 10787 -68
- Partials 200 221 +21
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| @property | ||
| def parameters(self) -> Optional[Dict[str, Any]]: | ||
| # Build parameters if db_engine_spec is a subclass of BasicParametersMixin | ||
| parameters = {"engine": self.backend} |
There was a problem hiding this comment.
so for sqlalchemy forms, we're only going to return engine? Maybe that should be the only required type then, it looks like.
There was a problem hiding this comment.
Yea good catch i'll make sure to update the type on the types file
| # Build parameters if db_engine_spec is a subclass of BasicParametersMixin | ||
| parameters = {"engine": self.backend} | ||
|
|
||
| if issubclass(self.db_engine_spec, BasicParametersMixin): |
There was a problem hiding this comment.
After we add BQ we need to change this, right?
There was a problem hiding this comment.
Yea that PR will have the updated logic here to allow us to pass Big Query credentials
|
closing in favor of #14653 |
SUMMARY
Update the api to surface parameters in GET
database. This will be leveraged for whenever a user wants to edit a database assuming configuration type is dynamic.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
ADDITIONAL INFORMATION